Adding Python 3 support #10
Conversation
Most of the changes were unnecessary, such as converting iterators into lists, or adding a dependency on futurize.
Most of the unchosen changes were the unnecessary addition of futurize as a dependency.
It's needed for Python 2/3 compatibility, but isn't included with six.
The exceptions are __init__.py, because __all__ is set to [], and in _py.py, launch_ipython_with_autoimporter is undefined, but I do not know what it should actually be (no such function is defined anywhere in pyflyby).
With the exception of those in test_autoimp.py, which are undefined names intentionally. If it was desired to make pyflakes pass 100% on all test files those functions could be created using exec().
This breaks pyflakes testing because there are variables that appear to be unused. When Python 2 support is dropped, these strings can become f-strings.
The code can only run in Python 2, so revert the incorrect change from futurize. This fixes 'py console' in Python 2.
This reverts some futurize changes. The tests are only relevant in Python 2, because they deal with oldstyle classes.
This reverts an incorrect futurize change, and makes it so it should work properly in both Python 2 and 3.
|
I guess Travis doesn't run on draft pull requests? That would make the feature rather useless. |
This allows the sorted() calls in autoimp to work in Python 3.
|
The b64decode examples in the |
|
Good point. Let’s pick examples that work the same in py2/py3.
Karl
On Apr 5, 2019, at 18:49, Aaron Meurer <[email protected]<mailto:[email protected]>> wrote:
The b64decode examples in the auto_eval doctest aren't very good, because it returns bytes in Python 3, producing different output (b'hello' vs. hello). Should I add .decode('utf-8') to the examples, or pick a better function that acts the same in both 2 and 3?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<#10 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AADDZ4pNxfqpthw-gctPAUNWcopCGMzzks5vd9LqgaJpZM4cftCa>.
|
|
I've gone through all the places in https://greentreesnakes.readthedocs.io/en/latest/nodes.html where Python 2/3 differences are noted and added tests for those node types. I still need to find some good codebases that use Python 3 features to run some smoke tests on. Although I already know of at least one error in the parser, also present in master from the pyflyby sources themself #12. |
This reverts commit 9bd9d73.
The file is only used in Python 2 and doesn't have prints. The extra line changes the docxref test.
It may be why tests are randomly failing on Travis again.
|
I guess this is ready for review. I'll try to think of some good Python 3 codebases I can test it against this weekend. Let me know if you have any suggestions. I'm not aware of any outstanding issues, except for the mandatory imports question, and the upstream fixes. |
|
I guess I forgot to mention here that there is another relevant upstream issue, ipython/ipython#11714. In the newest IPython, the input transformers are called multiple times. This results in the pyflyby messages being printed multiple times. The workaround for this isn't exactly pretty. I don't know how to fix the issue upstream, so we'll have to wait on the IPython guys to do something about it. |
lib/python/pyflyby/_autoimp.py
Outdated
| @@ -24,6 +25,8 @@ | |||
| from pyflyby._modules import ModuleHandle | |||
| from pyflyby._parse import PythonBlock, infer_compile_mode | |||
|
|
|||
| if PY2: | |||
| long = int | |||
There was a problem hiding this comment.
This seems less than ideal. Why not just change the lone long call to int?
There was a problem hiding this comment.
I'm sure I meant to do this differently, but since it apparently works as an int rather than a long, I'll just remove it.
lib/python/pyflyby/_flags.py
Outdated
| @@ -4,6 +4,7 @@ | |||
|
|
|||
| from __future__ import absolute_import, division, with_statement | |||
|
|
|||
| from __future__ import print_function | |||
There was a problem hiding this comment.
This should be combined with the above import. I don't recall if it works to have multiple time machine statements because it is handled in a special way.
| stack[3].function == 'run_cell_async', | ||
| # stack[3].function == 'should_run_async', | ||
| # stack[1].function == 'check_complete' | ||
| ]): |
There was a problem hiding this comment.
isn't this equivalent to just if stack[3].function == 'run_cell_async':
There was a problem hiding this comment.
Yea, I wanted to leave the other ones in there as comments, in case this isn't actually correct (as far as I can tell from the tests, it should be). I don't fully understand where the three different places are called. I originally thought the inverse of this was correct, but apparently it isn't.
There was a problem hiding this comment.
I think it is a little confusing to keep this as it is now. Can you please add a comment saying why the others are commented out and when to uncomment them. Also, you probably don't need the False in there.
There was a problem hiding this comment.
I think I added the False so that I could comment all three out (which is stupid because any([]) is the same as any([False])).
|
Thanks @asmeurer - just a couple of small requests |
It apparently doesn't matter if it is an int or a long.
setup.py collect_imports is currently broken because of deshaw#12, but I have added it to .pyflyby manually. I have not yet re-run tidy-imports on the codebase.
The tidy-imports added a line to the top of the file.
|
@quarl - I think this is good to go now. Should I merge it in? |
|
Great! Yes please. |
|
🎉 |
Fixes #2
This is the same as #9, but reopened so that Travis works and it hides the commits that are already in master.
TODOs:
__future__imports.pycommand line examples